Skip to content

drivers: stepper: introduce rudimentary trapezoidal ramp control - #88564

Closed
jilaypandya wants to merge 2 commits into
zephyrproject-rtos:mainfrom
jilaypandya:feature/introduce-rudimentary-ramp
Closed

drivers: stepper: introduce rudimentary trapezoidal ramp control#88564
jilaypandya wants to merge 2 commits into
zephyrproject-rtos:mainfrom
jilaypandya:feature/introduce-rudimentary-ramp

Conversation

@jilaypandya

@jilaypandya jilaypandya commented Apr 13, 2025

Copy link
Copy Markdown
Member

Trapezoidal Ramp Control for Stepper Motors:

The idea is to create a ramp lib that can be integrated in any driver, the step counting for the driver itself stays unaffected by integration a ramp lib

I have been using Shell to see how the ramping looks like.

Starting This PR to get some early feedback.

Integration in driver

Sequence Diagram

References:
https://www.boost.org/doc/libs/1_81_0/libs/safe_numerics/example/stepper-motor.pdf
Equation 13 in the above mentioned paper can also be seen in this app note.
https://ww1.microchip.com/downloads/en/Appnotes/doc8017.pdf (Thanks @andre-stefanov :) )


Known open points:

  • Integrate ramp control for stepper_run as well
  • use of sqrt in calculate_start_velocity, this function gets called once when the ramping starts from zero-velocity
    copied isqrt
  • Implement unit tests
  • Introduce DT based selection of trapezoidal ramp, allowing inclusion of other ramps in future

Update: 23.04.2025
Thanks @andre-stefanov for contributing unit-tests :)

Comment thread drivers/stepper/ramp/ramp.h Outdated
Comment thread drivers/stepper/ramp/ramp.h Outdated
Comment thread include/zephyr/drivers/stepper.h Outdated
Comment thread drivers/stepper/ramp/ramp_trapezoidal.c Outdated
Comment thread include/zephyr/drivers/stepper.h Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is max_velocity really needed? next step interval depends only on current interval and acceleration/deceleration values.

the actual limit will be given by the requested stepper interval

@jilaypandya jilaypandya Apr 13, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to depict a trapezoidal ramp. I am also using max_velocity to check for valid_timing in move_by and move_to. But i'll check this again for sure if it is actually needed :).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my experience, the speed set via set_microstep_interval is sufficient.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay we can use that function, but we definitely need to have a better name for it though. set_microstep_interval atleast imo sounds like this interval will take place with immediate effect. Probably something like set_target_step_interval ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the current micro-step interval implementation also requires changes to pre-decel-steps, as it is dependent on the speed. My recommendation would be to use the current micro-step interval in combination with the deceleration value for this

@jilaypandya
jilaypandya force-pushed the feature/introduce-rudimentary-ramp branch 4 times, most recently from 03d7e6a to 002919a Compare April 14, 2025 07:12
Comment thread include/zephyr/drivers/stepper.h Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my experience, the speed set via set_microstep_interval is sufficient.

Comment thread drivers/stepper/gpio_stepper_controller.c Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need max_velocity, you already have set_microstep_interval.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to find a consensus on how to rename that function, set_microstep_interval imo signifies more like a tick time, could be compared with for instance , can_set_bitrate.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say, once this PR is merged, the CONFIG_STEPPER_RAMP can be deleted and all motors should support ramping by default.

Comment thread drivers/stepper/ramp/Kconfig Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer a dt-binding instead to allow for a per stepper-controller decision on using ramps or not.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. However, you would need one such variable for the source to be included, or is there a better way?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a Kconfig extension for checking if a property is present on a compatible:

drivers/dma/Kconfig.mcux_edma:12:	depends on $(dt_compat_any_has_prop,$(EDMA_COMPAT),$(REV_PROP),2)

but nothing like dt_any_of_driver_class_has_prop, which would be neat.

Comment thread drivers/stepper/ramp/ramp_trapezoidal.c Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You cant always use the approximation. You will need to use the accurate algorithm using the sqrt for a number of steps after starting accelerating and a couple of steps before stopping when decelerating. This is because of the approximation error.

@jbehrensnx

Copy link
Copy Markdown
Contributor

My PR 88342 already uses the same algorithm (through I forgot to mention the algorithm admittedly) for a step-dir implementation. Several decisions made during development for that are relevant for this PR as well, so it should be included in the discussion.

@kartben kartben assigned jilaypandya and unassigned kartben Apr 14, 2025
Comment thread dts/bindings/stepper/ramp-parameters.yaml Outdated
@decsny
decsny removed their request for review April 14, 2025 17:04
@jilaypandya
jilaypandya force-pushed the feature/introduce-rudimentary-ramp branch from 002919a to 2fb84c6 Compare April 14, 2025 19:33
@dipakgmx

Copy link
Copy Markdown
Member

This is not related to the changes made in this PR, but could the functions move_to & move_by call either of them, I mean, in move_by, call move_to(current_pos+move_by_target)? It seems to me that some bits of code are repeated in both the functions (or thats what I felt when reviewing)

@jilaypandya
jilaypandya force-pushed the feature/introduce-rudimentary-ramp branch 2 times, most recently from f0f04fe to e5ab023 Compare April 20, 2025 16:01
Comment thread drivers/stepper/stepper_common.h Outdated
@jilaypandya
jilaypandya force-pushed the feature/introduce-rudimentary-ramp branch 2 times, most recently from ca14aeb to bdb0a32 Compare April 21, 2025 09:13
Comment thread samples/drivers/stepper/generic/Kconfig
/**
* @brief Stepper ramp profile
*/
struct stepper_ramp_profile {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not have this in stepper.h directly?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

coz then ramp.h would need to include stepper.h and i wanted to keep the ramp.h as decoupled from the stepper.h as possible.

Need some feedback on this actually, how about instead of stepper_set_ramp_profile(ramp_profile), we do something like stepper_set_timing_data(accel, max_velocity, decel) and then we put ramp_profile in ramp.h

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done moved to stepper.h

Comment thread drivers/stepper/ramp/Kconfig Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a Kconfig extension for checking if a property is present on a compatible:

drivers/dma/Kconfig.mcux_edma:12:	depends on $(dt_compat_any_has_prop,$(EDMA_COMPAT),$(REV_PROP),2)

but nothing like dt_any_of_driver_class_has_prop, which would be neat.

Comment thread drivers/stepper/ramp/ramp.h Outdated
*/
typedef void (*stepper_ramp_reset_ramp_runtime_data_t)(const struct stepper_ramp_config *config,
struct stepper_ramp_common *ramp_common,
const bool is_stepper_dir_changed,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it maybe make the API more stable when you move is_stepper_dir_changed etc. into the runtime data?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep that's correct :), Will do it in the next push.

@jilaypandya
jilaypandya force-pushed the feature/introduce-rudimentary-ramp branch from bdb0a32 to 1014b4c Compare April 23, 2025 08:08
@jilaypandya
jilaypandya force-pushed the feature/introduce-rudimentary-ramp branch 3 times, most recently from 12d8d44 to a4be436 Compare April 23, 2025 17:03
@jilaypandya jilaypandya changed the title drivers: stepper: introduce rudimentary ramp control drivers: stepper: introduce rudimentary trapezoidal ramp control Apr 23, 2025
Comment thread drivers/stepper/gpio_stepper_controller.c Outdated
@jilaypandya
jilaypandya force-pushed the feature/introduce-rudimentary-ramp branch 2 times, most recently from d263d8e to 314c9e9 Compare April 24, 2025 18:40

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Directly return this?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Return this directly

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@jilaypandya
jilaypandya force-pushed the feature/introduce-rudimentary-ramp branch from 314c9e9 to 0be7169 Compare April 25, 2025 14:33
@jilaypandya
jilaypandya force-pushed the feature/introduce-rudimentary-ramp branch from 0be7169 to 402bc78 Compare April 28, 2025 09:00

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The non ramp part uses a if DIRECTION_POSITIVE else if DIRECTION_NEGATIVE ordering and I would keep it consistent.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks. done

Comment thread drivers/stepper/ramp/ramp_trapezoidal.c Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
(2 * current_step_interval) / (4 * ramp_position + 1);
(2 * current_step_interval) / (4 * ramp_position + 3);

Using this, you effectively add 0.5 to ramp position, calculating the new interval based on the middle of the ramp segment and not the start, improving acceleration behavior. Note that your deceleration calculation already does this.

Comment thread drivers/stepper/ramp/ramp_trapezoidal.c Outdated
Comment thread drivers/stepper/ramp/ramp_trapezoidal.c Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an interval and not a velocity.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment thread include/zephyr/drivers/stepper.h Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the current micro-step interval implementation also requires changes to pre-decel-steps, as it is dependent on the speed. My recommendation would be to use the current micro-step interval in combination with the deceleration value for this

Comment thread tests/drivers/stepper/ramp/src/main.c Outdated
Comment thread tests/drivers/stepper/ramp/src/main.c Outdated
Comment thread tests/drivers/stepper/ramp/src/main.c Outdated
@jilaypandya
jilaypandya force-pushed the feature/introduce-rudimentary-ramp branch from 402bc78 to 8d25028 Compare April 28, 2025 09:06
jilaypandya and others added 2 commits April 28, 2025 11:15
Introduce rudimentary trapezoidal ramp control using following paper:
https://www.boost.org/doc/libs/1_81_0/libs/safe_numerics/example/stepper-motor.pdf

Signed-off-by: Jilay Pandya <jilay.pandya@outlook.com>
testing following criteria:
- distance profile
- first acceleration interval
- next step intervals

Signed-off-by: Andre Stefanov <mail@andrestefanov.de>
@jbehrensnx

Copy link
Copy Markdown
Contributor

As a general thing, in my experience, you need picosecond resolution for the ramp calculations, which are then converted to nanoseconds for the stepper controllers (but saving the picosecond values for the next step). Not sure if your 0.676 to counteract approximation inaccuracies reduces accuracy so much that this no longer matters.

Could could you maybe test if in an acceleration towards 50 or 200 steps/s in 1 sec actually takes 1 second? I remember during my development using this algorithm that I often got inaccuracies in the range of 20%, something that is not noticeable when using only the console without printing the times.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants